Skip to content

ref(snapshots): Add configurable diff_threshold for snapshot comparison#112728

Merged
NicoHinderling merged 2 commits intomasterfrom
04-10-ref_snapshots_add_configurable_diff_threshold_for_snapshot_comparison
Apr 10, 2026
Merged

ref(snapshots): Add configurable diff_threshold for snapshot comparison#112728
NicoHinderling merged 2 commits intomasterfrom
04-10-ref_snapshots_add_configurable_diff_threshold_for_snapshot_comparison

Conversation

@NicoHinderling
Copy link
Copy Markdown
Contributor

@NicoHinderling NicoHinderling commented Apr 10, 2026

Summary

  • Add diff_threshold (0.0–1.0) parameter to the snapshot upload API, allowing callers to control what percentage of changed pixels marks an image as "changed"
  • Plumb the threshold from the API endpoint through the manifest to the comparison task
  • Clean up dead CLI retry logic in compare.py

Test plan

  • Existing image diff tests updated and passing
  • New dimension-mismatch tests cover padding behavior
  • Schema validates diff_threshold type and range at the API boundary

Copy link
Copy Markdown
Contributor Author

NicoHinderling commented Apr 10, 2026

@github-actions github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label Apr 10, 2026
@NicoHinderling NicoHinderling marked this pull request as ready for review April 10, 2026 19:42
@NicoHinderling NicoHinderling requested a review from a team as a code owner April 10, 2026 19:42
Comment thread src/sentry/preprod/snapshots/tasks.py Outdated
Comment thread src/sentry/preprod/snapshots/tasks.py
Comment thread src/sentry/preprod/snapshots/tasks.py
Comment thread src/sentry/preprod/snapshots/tasks.py
@NicoHinderling NicoHinderling force-pushed the 04-10-ref_snapshots_add_configurable_diff_threshold_for_snapshot_comparison branch from 2a40e7f to df7a12d Compare April 10, 2026 21:19
@NicoHinderling NicoHinderling force-pushed the 04-09-ref_snapshots_make_diffing_logic_more_strict_better_handle_different_sized_images_when_comparing branch from 96dfb80 to 199b40c Compare April 10, 2026 21:19
Base automatically changed from 04-09-ref_snapshots_make_diffing_logic_more_strict_better_handle_different_sized_images_when_comparing to master April 10, 2026 21:30
@NicoHinderling NicoHinderling force-pushed the 04-10-ref_snapshots_add_configurable_diff_threshold_for_snapshot_comparison branch from df7a12d to 418c4ee Compare April 10, 2026 21:30
Comment thread src/sentry/preprod/api/endpoints/preprod_artifact_snapshot.py Outdated
…ound

Make diff_threshold exclusive of 1.0 in both the API schema and
Pydantic model since a threshold of 1.0 with strict > comparison
would silently prevent any image from being marked changed. Also
remove the hash-mismatch fallback that forced changed status when
odiff reported 0 diff — the upstream odiff bug is being fixed.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Copy link
Copy Markdown
Contributor

@cursor cursor bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 2fdde05. Configure here.

else 0
)
effective_threshold = diff_threshold if diff_threshold is not None else 0.0
is_changed = diff_pct > effective_threshold
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing hash-mismatch safety check described in PR

Medium Severity

The PR description explicitly states "Add a hash-mismatch safety check: if odiff reports 0 diff but content hashes differ, force the image as changed," and the PR discussion shows a hashes_differ = head_hash != base_hash line that was present in an earlier version of the code. This safety check is missing from the final diff. At this point in the code, head_hash != base_hash is guaranteed (filtered at line 414), but odiff can still report 0 changed_pixels due to its own ODIFF_SENSITIVITY_DIFF_THRESHOLD. Without the safety check, such images are incorrectly classified as "unchanged" even though their content hashes differ.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 2fdde05. Configure here.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

forgot to update PR description

@NicoHinderling NicoHinderling merged commit a3f9c2c into master Apr 10, 2026
57 checks passed
@NicoHinderling NicoHinderling deleted the 04-10-ref_snapshots_add_configurable_diff_threshold_for_snapshot_comparison branch April 10, 2026 21:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Scope: Backend Automatically applied to PRs that change backend components

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants